-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: Stop reordering arguments throwing distinct exceptions #70893
Conversation
Implement a precise scan for arguments that may throw exceptions which collects the exceptions they may throw. When we see two such interfering sets, make sure that the first argument is evaluated to a temp so that they do not get reordered by sort. Fix dotnet#63905
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsImplement a precise scan for arguments that may throw exceptions which Fix #63905 Also optimistically see if this is enough to relax some of the forward sub constraints...
|
This reverts commit 1536b67. Will do this separately in a follow-up change.
cc @dotnet/jit-contrib PTAL @AndyAyersMS Diffs. Some small regressions and a minor TP impact, as expected. |
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
Ensure we do not set stack args as needing a temp.
jitstress runs hit a bunch of #71023 so will retry. |
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
Does allowing this slight amount of reordering in the face of exceptions provide benefits over not allowing any reordering at all? I get that it would be very difficult for end users to figure out which arg tree is responsible, especially so if the tree can't have global effects, but I wonder what the difference is between this and just relying on |
Yes, this results in larger regressions (it was the first thing I tried): aspnet.run.windows.x64.checked.mch:
Detail diffs
benchmarks.run.windows.x64.checked.mch:
Detail diffs
coreclr_tests.pmi.windows.x64.checked.mch:
Detail diffs
libraries.crossgen2.windows.x64.checked.mch:
Detail diffs
libraries.pmi.windows.x64.checked.mch:
Detail diffs
libraries_tests.pmi.windows.x64.checked.mch:
Detail diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM.
Implement a precise scan for arguments that may throw exceptions which
collects the exceptions they may throw. When we see two such interfering
sets, make sure that the first argument is evaluated to a temp so that
they do not get reordered by sort.
Fix #63905
I think there is still low hanging fruit in optimizing the sort itself but I am focusing on fixing the correctness issues here.
I expect once this is merged we can loosen up some of the forward sub restrictions around call args.